Zoom menu item and other minor fixes#692
Conversation
| enabled = enabled, | ||
| modifier = Modifier.testTag(TestTags.ZOOM_PERCENTAGE) | ||
| ) { | ||
| Text("${(zoomRatio * 100).roundToInt()}%") |
There was a problem hiding this comment.
Seems better to use java.text.NumberFormat which has getPercentInstance for percentage formatting
There was a problem hiding this comment.
The formatting is using NumberFormat.getPercentInstance now.
| TextButton( | ||
| onClick = onCustomZoom, | ||
| enabled = enabled, | ||
| modifier = Modifier.testTag(TestTags.ZOOM_PERCENTAGE) |
There was a problem hiding this comment.
Could add .semantics { liveRegion = LiveRegionMode.Polite } for accessibility
| }, | ||
| singleLine = true, | ||
| isError = textFieldValue.text.isNotEmpty() && !isValid, | ||
| suffix = { Text("%") }, |
There was a problem hiding this comment.
There are some locales where the % is placed at the front but is still LTR, e.g. Turkish. Although we don't have general localization support, it might be better to be more future proof
There was a problem hiding this comment.
I used a boolean switch for this one and get the direction from NumberFormat. The most correct solution seems to be using VisualTransformation but for this simple input that is probably overkilling.
| override fun onZoom(scaleFactor: Float, focusX: Float, focusY: Float) { | ||
| viewModel.zoomRatio = (viewModel.zoomRatio * scaleFactor) | ||
| .coerceIn(MIN_ZOOM_RATIO, MAX_ZOOM_RATIO) | ||
| viewModel.setZoomRatio( | ||
| (viewModel.zoomRatio.value * scaleFactor) | ||
| .coerceIn(MIN_ZOOM_RATIO, MAX_ZOOM_RATIO) | ||
| ) | ||
| viewModel.zoomFocusX = focusX |
There was a problem hiding this comment.
Seems to be an issue with cached zooming, e.g.
- Set zoom to 200% in the dialog
- Press preset to increase to 300%, then go back to 200%
- Scroll away from the page origin, then begin pinching outward to zoom in
- Observe the page jump or pan toward the wrong location as the pinch begins instead of retaining the content beneath the fingers
pdfzoom-1.mp4
There was a problem hiding this comment.
This is fixed now.
| val documentProperties by viewModel.documentProperties.collectAsStateWithLifecycle() | ||
| val outlineStatus by viewModel.outline.collectAsStateWithLifecycle() | ||
| val showPasswordDialog by viewModel.showPasswordDialog.collectAsStateWithLifecycle() | ||
| val zoomRatio by viewModel.zoomRatio.collectAsStateWithLifecycle() |
There was a problem hiding this comment.
This seems to be causing the PdfTopAppBar to recompose itself on every single zoom ratio change; simply zooming in and out caused it to recompose 100s of times
This can be observed in the Layout Inspector in Android Studio using Running Devices button on the right menu > Press + to add device > Top right to toggle Layout Inspector, then do pinch zooming
ScaleGestureDetector.onScale() runs repeatedly throughout the gesture. Each distinct ratio now:
- updates the
StateFlow; - schedules recomposition from the screen-level state read;
- passes a changed
FloatintoPdfTopAppBar, preventing that subtree from being skipped; - independently issues
evaluateJavascript("onRenderPage(2)").
This work occurs even when the overflow menu is closed and the zoom percentage is not visible. It theoretically adds Compose work on the same main thread hot path responsible for interactive pinch rendering
We should observe the zoom flow at the narrowest subtree that displays it, ideally inside dropdown content that is only composed while expanded
There was a problem hiding this comment.
You are right, I didn't realize the compose is happening so many times. The flow is moved to be inside ZoomRow now.
- Add a menu item for zooming, and a dialog for entering numbers when clicked - Fix wrong text color on WebView crash screen - Update icons to use Material Symbols
- Fix position drift bug - Move zoom flow into ZoomRow - Make percentages and number formats respect locale
2 participants
Closes #569